-
Notifications
You must be signed in to change notification settings - Fork 24.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow plugins to upgrade global custom metadata on startup #19962
Conversation
* Returns a function that upgrades metadata on startup. | ||
* Plugins should return the input metadata if no upgrade is required | ||
* and throw {@link IllegalStateException} to stop a node from starting | ||
* when unsupported metadata is found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest more official Javadoc:
/**
* Provides a function to modify the metadata on startup.
* <p>
* Plugins should return the input metadata via {@link Function#identity()} if no upgrade is required.
* @return Never {@code null}. The same or upgraded {@code MetaData}.
* @throws IllegalStateException if the node needs to be stopped because {@code MetaData} is unsupported
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the docs, added :)
The change looks good, but we should add some tests to ensure it consistently happens. |
|
||
@Nullable | ||
private volatile MetaData previousMetaData; | ||
|
||
private volatile Set<Index> previouslyWrittenIndices = emptySet(); | ||
|
||
@Inject | ||
public GatewayMetaState(Settings settings, NodeEnvironment nodeEnv, MetaStateService metaStateService, | ||
public GatewayMetaState(Settings settings, NodeEnvironment nodeEnv, PluginsService pluginsService, MetaStateService metaStateService, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the way to get the custom upgraders passed around (we should not be passing around PluginService). See other examples in Node: you should collect all the upgraders, and put them into a container that will then be injected (and eventually when GatewayMetaState is deguiced, we will just have it there as an arg, instead of via injection).
This pull request is missing tests. Some possible test cases:
|
Regarding two plugins upgrade, the plugins should not be modifying the same parts of metadata (this is really for custom metadata). Another suggested test is to check upgrading actually works (ie is it called at all for plugins which add an upgrader). |
Yes, but a test should check that both pieces of custom metadata are updated when two plugins upgrade on startup. |
57186d5
to
d99bc98
Compare
Thanks @pickypg, @jasontedor & @rjernst for the feedback, I have restructured the code and added unit tests upgrading. Could you take another look?
I wonder, if we need an explicit test for this in light of the current changes? We collect the functions just like we collect the namedWritables and components |
* <p> | ||
* Plugins should return the input metadata via {@link Function#identity()} if no upgrade is required. | ||
* @return Never {@code null}. The same or upgraded {@code MetaData}. | ||
* @throws IllegalStateException if the node needs to be stopped because {@code MetaData} is unsupported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say something like "the node should not start" instead of the part about stopping (since this is during startup, while stopping implies the node is already started).
Thanks @areek, I left some more comments. I think the tests should actually be on GatewayMetaState. And I think there may be a subtle bug regarding that: we need to be careful in there about only loading the metastate from disk once, passing it through all of the methods to manipulate/check it, and then writing it once all checks have passed. Otherwise, for example, right now index upgrading would happen, we write the new state, the plugged in metadata converters run, and if one fails, the node cannot be downgraded. |
e9139ad
to
ee06b4a
Compare
@rjernst Thanks for pointing out that node cannot be downgraded, if plugins error out on upgrade. I have separated out logic to create upgraded meta data and writing them to disk. Now plugins can validate |
@areek I think you misunderstood what I meant. I was suggesting GatewayMetaState should be modified, so that it reads the metastate at the beginning, runs it through all modifications/checks, and then writes it at the end of the ctor. I don't think we need two different interfaces for upgrade/validation. We just need to be more careful in the class calling all of these. |
@rjernst this is what it does. Now, all the upgrading logic is in I agree that you can still do the validation on |
I don't like the validation separate because it doesn't match the purpose of the upgrade step. There should be no "validation only" possible. Think of it as upgrading is happening and it either succeeds with new meta state written, or fails with the node shutting down. |
hmm, ok, so we should do the validation in |
Yes, all I mean is to have one method on Plugin. |
ee06b4a
to
4236c0f
Compare
4236c0f
to
f5f2811
Compare
I removed |
f5f2811
to
4c34896
Compare
customs -> { | ||
List<MetaData.Custom> customList = new ArrayList<>(); | ||
for (MetaData.Custom custom : customs) { | ||
if (custom.type().equals(CustomMetaData1.TYPE)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice this pattern in every implementation. Perhaps this should be a Map instead of Collection (keyed by the custom type name)? Then the map can be copied, and keys replaced, removed, or added easily, without needing to have logic for the other custom metadata that the plugin does not care about.
Thanks @areek. It is looking better. I left a couple more suggestions. |
4c34896
to
97b2546
Compare
Thanks @rjernst for the suggestions, addressed all your feedback. |
97b2546
to
c353ba9
Compare
LGTM |
Currently plugins can not inspect or upgrade custom meta data on startup. This commit allow plugins to check and/or upgrade global custom meta data on startup. Plugins can stop a node if any custom meta data is not supported.
c353ba9
to
75d4a9f
Compare
Currently plugins can not inspect or upgrade custom
meta data on startup. This commit allow plugins
to check and/or upgrade global custom meta data on startup.
Plugins can stop a node if any custom meta data is not supported.